Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added non-agent-specific docs on span compression #6438

Merged
merged 9 commits into from
Nov 25, 2021
Merged

Added non-agent-specific docs on span compression #6438

merged 9 commits into from
Nov 25, 2021

Conversation

beniwohli
Copy link
Contributor

Motivation/summary

The agent team felt that the span compression feature requires some documentation that is not agent specific. Similar to the sampling docs, it seems like the APM Server docs is the best place for this.

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

Related issues

elastic/apm#432
#5485

@beniwohli beniwohli requested a review from bmorelli25 October 27, 2021 14:22
@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2021

This pull request does not have a backport label. Could you fix it @beniwohli? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.x is the label to automatically backport to the 7.x branch.
  • backport-7./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Oct 27, 2021
@beniwohli beniwohli marked this pull request as draft October 27, 2021 14:25
@beniwohli beniwohli mentioned this pull request Oct 27, 2021
Closed
@apmmachine
Copy link
Contributor

apmmachine commented Oct 27, 2021 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-23T19:05:32.009+0000

  • Duration: 42 min 57 sec

  • Commit: 47fc40b

Test stats 🧪

Test Results
Failed 0
Passed 5582
Skipped 19
Total 5601

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /hey-apm : Run the hey-apm benchmark.

  • /package : Generate and publish the docker images.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

bmorelli25
bmorelli25 reviewed Oct 28, 2021
View reviewed changes
Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the APM Server Reference and APM Overview are going away in 7.16 to make room for a new APM Guide book. This book will contain content from both the Server Reference and Overview that is relevant to the APM integration.

I hooked this page into the "features" bucket in the new APM Guide. Let me know if you think that's an okay place for this content to live.

docs/span-compression.asciidoc Outdated Show resolved Hide resolved
docs/span-compression.asciidoc Outdated Show resolved Hide resolved
docs/span-compression.asciidoc Outdated Show resolved Hide resolved
docs/span-compression.asciidoc Outdated Show resolved Hide resolved
docs/span-compression.asciidoc Outdated
Comment on lines 57 to 58
* Go
* Python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have docs to link to yet? I did a quick search in each agent's book and couldn't find anything.

marclop
marclop reviewed Oct 29, 2021
View reviewed changes
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks good, I think we should also include the conditions that would make a span compressible, regardless of the compression strategy.

docs/span-compression.asciidoc Outdated Show resolved Hide resolved
@beniwohli @bmorelli25 @marclop
Apply suggestions from code review
d31c0bd 
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
@simitt
Copy link
Contributor

simitt commented Nov 22, 2021

@beniwohli can we help to get this PR out of Draft state or is it blocked by something?

@beniwohli
Copy link
Contributor Author

@simitt totally forgot that it is still in draft, thanks for the heads up. I'll give it a last read through this morning and set it to ready for review afterwards 👍

@beniwohli beniwohli marked this pull request as ready for review November 22, 2021 15:37
@beniwohli
Copy link
Contributor Author

The references to the Go agent docs break because the feature is not released yet and hence only available in the master docs, not current. I'm not sure how to handle this properly.

@simitt
Copy link
Contributor

simitt commented Nov 22, 2021

@bmorelli25 could you help out on this issue please.

The references to the Go agent docs break because the feature is not released yet and hence only available in the master docs, not current. I'm not sure how to handle this properly.

@bmorelli25 bmorelli25 mentioned this pull request Nov 23, 2021
Closed
@bmorelli25
Copy link
Member

Pushed the fix and opened elastic/observability-docs#1285 to update my workaround when the next version of the Go Agent releases.

@bmorelli25 bmorelli25 added backport-7.16 Automated backport with mergify to the 7.16 branch backport-8.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Nov 23, 2021
@bmorelli25 bmorelli25 merged commit 9ebd95e into elastic:master Nov 25, 2021
mergify bot pushed a commit that referenced this pull request Nov 25, 2021
@beniwohli
Added non-agent-specific docs on span compression (#6438)
6c6f2b7 
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
(cherry picked from commit 9ebd95e)
@mergify mergify bot mentioned this pull request Nov 25, 2021
Merged
mergify bot pushed a commit that referenced this pull request Nov 25, 2021
@beniwohli
Added non-agent-specific docs on span compression (#6438)
2fbd0f1 
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
(cherry picked from commit 9ebd95e)
@mergify mergify bot mentioned this pull request Nov 25, 2021
Merged
bmorelli25 pushed a commit that referenced this pull request Nov 25, 2021
@mergify @beniwohli
Added non-agent-specific docs on span compression (#6438) (#6716)
5ccf895 
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
(cherry picked from commit 9ebd95e)

Co-authored-by: Benjamin Wohlwend <[email protected]>
bmorelli25 pushed a commit that referenced this pull request Nov 25, 2021
@mergify @beniwohli
Added non-agent-specific docs on span compression (#6438) (#6717)
7952c14 
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Brandon Morelli <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
(cherry picked from commit 9ebd95e)

Co-authored-by: Benjamin Wohlwend <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marclop marclop marclop left review comments

@bmorelli25 bmorelli25 bmorelli25 approved these changes

Assignees
No one assigned
Labels
backport-7.16 Automated backport with mergify to the 7.16 branch backport-8.0 Automated backport with mergify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@beniwohli @apmmachine @simitt @bmorelli25 @marclop